.

Re-review after rebase (v2)

Thanks for rebasing onto current main — the branch is now at 0 commits behind, merge conflicts are gone, and the commit count dropped from 14 to 10 (cleaner history). This resolves all 6 critical blockers and all 4 behavioral regressions from my first review:

✅ Resolved from v1 review

1. agent.* removed from setuptools → Fixed — agent.* is now preserved
2. Gateway hook system gutted → Resolved by rebase — the emit_collect() hook system remains untouched
3. user_name/chat_id removed from AIAgent → Resolved by rebase — no longer touched
4. release_gateway_runtime_lock() removed → Resolved by rebase
5. Plugin slash commands removed from Discord → Resolved by rebase
6. Transport cache invalidation removed → Resolved by rebase
7. MEDIA pattern extensions removed → Resolved by rebase
8. iOS smart-dash removal → Resolved by rebase
9. Discord _slash_commands config removed → Resolved by rebase
10. Auto-prune removed → Resolved by rebase

The diff is now much cleaner. Here's what remains:

Remaining concerns (non-blocking but worth discussing)

1. Soft dep → hard crash policy (still present)

These changes are still in the PR:

• Pillow: vision_tools.py, clipboard.py, tui_gateway/server.py — previously degraded gracefully (skip resize, fallback to ImageMagick or raw data URL), now crashes with ImportError
• tiktoken: tools_config.py — previously returned empty dict when unavailable, now crashes
• mutagen: discord.py — previously fell through to default 5s duration, now crashes
• psutil: mcp_tool.py — previously returned empty set, now crashes
• soundfile: neutts_synth.py — previously fell back to _write_wav() manual implementation, now crashes
• ptyprocess: process_registry.py — previously fell back to pipe mode with a warning, now crashes

Declaring these in extras ([cli], [messaging], [mcp], [pty], [tts-local]) is good. But the behavioral change from graceful degradation → crash is a separate decision from type cleanup.

Specific concern: The ptyprocess one is particularly risky — the current fallback to pipe mode is intentional and useful. Users who request PTY but don't have ptyprocess installed currently get pipe mode as a reasonable fallback. The crash means PTY requests will hard-fail instead.

Specific concern: The psutil one in _snapshot_child_pids() previously had a broader except Exception fallback that returned an empty set even for non-import failures. The new code catches psutil.Error but if psutil raises something else (e.g., permission error on macOS), it would propagate uncaught. The old fallback-to-empty-set was defensive for good reason.

Specific concern: The soundfile one removes a working _write_wav() fallback — this was real functionality, not silent degradation.

2. read_credential_pool() API change

The function signature changes from (provider_id: Optional[str] = None) to (). A new read_provider_credentials(provider_id: str) is added. The two internal callers are updated correctly. However, read_credential_pool is a public function in hermes_cli.auth — any plugin or external code calling read_credential_pool("openrouter") will now silently get the entire pool dict instead of the provider's entries (the parameter is gone, not rejected). Consider keeping the old signature with a deprecation warning, or at minimum documenting this as a breaking change.

3. api_server.py — aiohttp direct import at module level

The AIOHTTP_AVAILABLE guard is removed and aiohttp is imported unconditionally at module level. The check_api_server_requirements() function is deleted. This is fine because gateway/run.py now does import aiohttp with try/except before importing APIServerAdapter, so the module won't be imported when aiohttp is missing. The guard in run.py is the correct place for it.

However, the connect() method also had a guard: if not AIOHTTP_AVAILABLE: return False — this is now gone. This is fine given the run.py pre-check, but it does mean if someone imports APIServerAdapter directly without the pre-check, they'll get a module-level ImportError rather than a graceful False from connect().

4. _message_handler None guard in base.py

handler = self._message_handler
if handler is None:
    return
response = await handler(event)

This is a correct ty fix but changes behavior: previously await self._message_handler(event) would raise TypeError: 'NoneType' is not callable if _message_handler was None. Now it silently returns. This is arguably better but could mask initialization bugs.

5. _handle_retry_command return type widened to Optional[str]

-    async def _handle_retry_command(self, event: MessageEvent) -> str:
+    async def _handle_retry_command(self, event: MessageEvent) -> Optional[str]:

Need to verify callers handle None returns. The gateway dispatch generally does handle None (treats it as "no response"), so this is likely safe.

6. Discord hasattr → isinstance changes in gateway/run.py

Multiple hasattr(adapter, "method") checks are replaced with isinstance(adapter, DiscordAdapter). This is correct per our Pitfall #14 (MagicMock fools hasattr), but it adds from gateway.platforms.discord import DiscordAdapter imports inside each method. The imports are function-level (lazy), so no circular import risk. This is a good change.

7. Config TypedDicts (+361 lines in config.py)

Still present. These add maintenance burden — every new config key needs a TypedDict update. The total=True default means missing keys are type errors. Given how fast config evolves, this will require constant upkeep. Not blocking, but worth considering if the team is willing to maintain these.

8. Script relocation (batch_runner.py → scripts/)

Still present. The py-modules in pyproject.toml is updated (removes batch_runner, trajectory_compressor, rl_cli), scripts is added to packages.find.include, and scripts/__init__.py is created. Logger name changes in cli.py and run_agent.py are correctly updated. Test references are updated. The datagen-config-examples/ scripts are updated. This looks complete.

9. Minor nit: ty: ignore comments

The PR adds # ty: ignore[invalid-return-type] and # ty: ignore[invalid-key] comments in discord.py. These are for a non-standard type checker. If the project doesn't use ty in CI, these are just noise. But they don't hurt anything.

✅ Good changes that look correct

All the items from my v1 "salvageable" list are still present and correct:

• callable → Callable[..., Any] across run_agent.py, cli.py, delegate_tool.py, etc.
• os.sys.modules → sys.modules typo fix
• Discord null-safety guards (interaction.data, thread_id, member_count)
• Telegram sticker None guard
• Matrix walrus-narrowing fix
• build_anthropic_client(timeout: Optional[float]) widening
• OMIT_TEMPERATURE sentinel guard → kwargs-dict pattern
• CredentialPool.remove_entry() method
• _setup_wecom_callback() addition
• _CamofoxConfig TypedDict
• datetime.utcnow() → datetime.now(timezone.utc)
• email.py send_image/send_document signature fixes
• display.py start_time assert
• cron/scheduler.py None-safe error extraction
• skill_utils.py tuple unpacking fix
• image_generation_tool.py fal_client type narrowing (properly typed getattr results)
• process_registry.py pty import restructuring (though the no-fallback concern above)
• conversation_history: list | None widening
• base.py unreachable assertions after retry loops
• Various return type widenings (get_toolset_info, _handle_retry_command, _prepare_command, etc.)
• Tests for remove_entry, config shapes, RunState fields

Summary

The rebase resolved all the critical and behavioral issues. What remains is:

1. The soft→hard crash policy — the biggest philosophical question. Worth a team discussion on whether graceful degradation or fail-loud is preferred for optional deps.
2. Minor API breakage — read_credential_pool() signature change
3. Maintenance burden — config TypedDicts

The type fixes, null-safety guards, and structural additions (remove_entry, _setup_wecom_callback, RunState fields) are all valuable. If the team is comfortable with the soft→hard crash policy, this is much closer to mergeable. If not, those specific changes could be reverted while keeping everything else.